@brianhelba Thanks for the suggestions. I can definitely make these changes, but I'll first ask if we should make the same changes in the following places:

• setPinned function in the collection detail template
• collectionSetPinned api endpoint

I had copied code from each of these places so I could mimic the pinning behavior of collections. Should we try to keep them the same?

Good point. Since this is already getting to be a larger PR, I'd say is actually fine to keep here as-is, then make a follow-up PR to improve all these places.

@annehaley It's been a while since I've looked at the cursor pagination implementation but I'd really like to minimize changes to the paginator if at all possible. Is it possible to just order the queryset in the API view based on the query string and let the existing paginator handle it?

As it stands I don't think this is working at the moment. I think mutating state on the paginator is making the server stateful, which is leading to broken behavior e.g. request 1 orders by pinned and request 2 doesn't specify an ordering, but gets the same ordering as request 1. I believe the tests are passing by accident; for instance if I put this block after your first assertion:

    # List endpoint w/ no ordering
    r = authenticated_client.get(reverse("api:image_list"))
    ordered_ids = [image.get("isic_id") for image in r.json().get("results")]
    assert ordered_ids == [image_1.isic_id, image_2.isic_id]

It fails, but if I put it before, then it passes.

I think we actually ought to hardcode this to sorting by our pinned images ordering. If we allow users to arbitrarily order on anything then we run in to the risk of them ordering on something that isn't indexed, which would end up needing to do full table scans repeatedly. In this case, I think we might even be allowing users to introduce joins e.g. ordering by shares which could duplicate results and cause a lot of strain on the database.

So could we figure out how to aggressively pare back these changes, verify the pinned ordering is able to benefit from an index (it may not need a new one?), and see if it can work?

Ok, if we can assume that we always want to sort images pin-first then by creation timestamp, we no longer need the DynamicOrderCursorPagination class. It only exists to allow the user to specify the ordering in the query params. I had added this because you had orginally stated that the pin-first sort should be limited to opt-in via the API. Instead, we can just change the ordering declared on the Image model's Meta class.

However, we will still need to change the original CursorPagination class. The way it was built only considers ordering by one field. In these 2 places x, x, the code only considers the field at index 0 in the ordering. 6d95c5d updates the class to consider more than just the first field in the ordering list. Without this change, the paginator will only sort by pinned status without then ordering by creation timestamp.

@danlamanna As per our discussion, 3634342 isolates the custom pagination logic to a class called PinnedFirstPagination (open to other name suggestions). This class will still only order by created by default, but if the query contains the pin_sort parameter, then the ordering will be by pinned and then by created. This prevents arbitrary ordering, but still limits the API pin sorting to opt-in behavior. And rather than setting self.ordering, which caused the statefulness bug you identified, this class sets queryset.query.order_by when pin sorting is enabled.